Repository navigation
Fix #15144 FN knownConditionTrueFalse (binary or) - #8967
chrchr-github wants to merge 3 commits into
Conversation
b3382da to
9df4447
Compare
dmcppcheck
left a comment
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
The PR introduces a regression in min/max handling for unsigned types. It changes if (Token::simpleMatch(tok, "?") ... to else if in valueFlowImpossibleValues. For an unsigned ?: expression, the preceding astIsUnsigned(tok) branch now consumes the token, so the min/max symbolic impossible values are no longer set. Because of this, unsigned m = a < b ? a : b; if (m > a) {} loses its knownConditionTrueFalse warning. The signed version is still reported. The else should be reverted, since it isn't needed for #15144.
There is also a lesser issue. The new impossible-0 value on | survives conversion to a narrower type, which gives new false positives such as unsigned char c = i | 0x100; if (c) {} and int c = ll | 0x100000000LL; if (c) {}. In both cases the truncated value can be 0. Base already has the same weakness for a plain != 0 that gets narrowed. However, this PR makes it reachable through a common flag-setting idiom, so it may be worth limiting the new value to cases where the non-zero operand fits in the result type.
The added cases (i | 1, i | m after m == 0 returns) work as intended. I found no crashes.
Testing
Both builds succeeded: make -j8 cppcheck testrunner in pr/ and make -j8 cppcheck in base/.
Unit tests: ./testrunner TestCondition TestValueFlow and a full ./testrunner both pass (5351 tests, 0 failures).
Manual comparison (base vs PR): I ran cppcheck -q --enable=style on scratch files. The new true positives work as intended: int c = i | 1; if (c), if (m > 0) { int c = i | m; if (c) }, if (m < 1) return; if ((i | m) != 0) and i | (p != 0) after a null check. if (i | 1) still produces only the badBitmaskCheck warning, with no duplicate.
Regression (false negative) caused by if → else if:
unsigned d(unsigned a, unsigned b) { unsigned m = a < b ? a : b; if (m > a) {} return m; }
int e(int a, int b) { int m = a < b ? a : b; if (m > a) {} return m; }- base:
t1.cpp:4:72: style: Condition 'm>a' is always false [knownConditionTrueFalse]and the same for line 5 - PR: only the line 5 (signed) warning remains; the unsigned line 4 warning is gone.
New false positives from narrowing:
void a(int i) { unsigned char c = i | 0x100; if (c) {} }
void b(int i) { if ((unsigned char)(i | 0x100)) {} }
void c(long long i) { int c = i | 0x100000000LL; if (c) {} }- PR: reports
Condition 'c' is always truefor lines 1 and 3 andCondition '(unsigned char)(i|0x100)' is always truefor line 2. In all three cases the value can actually be 0. - base: reports nothing.
The same narrowing false positive already exists in base for if (i == 0) return; unsigned char c = i; if (c) {}, so this is an existing weakness that the PR exposes more often.
Automated review of 9df4447 by Claude. The pull request was built and tested locally.
| setTokenValue(tok, std::move(value), settings); | ||
| } | ||
| if (Token::simpleMatch(tok, "?") && Token::Match(tok->astOperand1(), "<|<=|>|>=")) { | ||
| else if (Token::simpleMatch(tok, "?") && Token::Match(tok->astOperand1(), "<|<=|>|>=")) { |
There was a problem hiding this comment.
AI review
This else causes a regression. An unsigned ?: token matches the preceding astIsUnsigned(tok) && !astIsPointer(tok) branch, so the min/max handling below is skipped for it. Example: unsigned f(unsigned a, unsigned b) { unsigned m = a < b ? a : b; if (m > a) {} return m; } reports Condition 'm>a' is always false on base but nothing with this PR. The signed version is still reported. #15144 doesn't need this change, so I suggest reverting it to if.
| value.bound = ValueFlow::Value::Bound::Lower; | ||
| value.setImpossible(); | ||
| setTokenValue(tok, std::move(value), settings); | ||
| } else if (tok->isBinaryOp() && tok->str() == "|") { |
There was a problem hiding this comment.
AI review
The impossible 0 is kept when the result is converted to a narrower type, so these are now reported as always true although they can be 0: unsigned char c = i | 0x100; if (c) {}, if ((unsigned char)(i | 0x100)) {} and long long i; int c = i | 0x100000000LL; if (c) {}. Base already has this weakness for a plain != 0 value that is narrowed, but x | FLAG is a common pattern, so it would help to only set the value when a known non-zero operand fits in the type it is converted to (or at least the lower bits).
No description provided.